/cubebot review

Claude finished @fslongjin's task in 1m 10s —— View job

PR Review: fix(network-agent): allow dots in sandboxID validation

• Read and understand the code change
• Review for correctness, security, and consistency
• Post review feedback

Verdict: LGTM — the fix is correct and well-aligned with existing patterns

The change (state_store.go:95):

// Before:
if strings.ContainsAny(sandboxID, `/\.`) || sandboxID == "" {

// After:
if sandboxID == "" || strings.ContainsAny(sandboxID, `/\`) || strings.Contains(sandboxID, "..") {

Correctness

The fix correctly distinguishes between a single . (safe, used in version strings like 3.11) and .. (path traversal). The original ContainsAny(sandboxID, /.) treated any dot as dangerous, which broke IDs like cubesandbox-python-slim-3.11-nydus_0.

Consistency with codebase

This now matches exactly the pattern used in Cubelet/pkg/pathutil/validate.go:17:

if strings.ContainsAny(id, `/\`) || strings.Contains(id, "..") {

It also aligns with Cubelet/pkg/utils/pathsec.go:21-22 which rejects ., .., and .. substrings:

if strings.ContainsAny(untrusted, `/\`) || untrusted == "." || untrusted == ".." ||
    strings.Contains(untrusted, "..") {

One subtle difference: pathsec.SafeJoinPath additionally rejects the bare . component (untrusted == "."). The state_store.path() function doesn't reject a bare . sandboxID. This is also consistent with pathutil.ValidateSafeID() which doesn't reject bare . either. Since the path() function appends .json, a bare . would produce <dir>/.json — a hidden file, not a traversal. This is a pre-existing behavior and not a regression introduced by this PR.

Security

Path traversal protection remains intact:

• / and \ are still rejected (path separators)
• .. is still rejected (traversal sequence)
• Single . in version strings like 3.11 is correctly allowed

Suggestion (non-blocking)

The existing state_store_test.go only tests the happy path. Adding a few validation test cases would prevent regressions and document the expected behavior:

func TestStateStorePathValidation(t *testing.T) {
    store, _ := newStateStore(t.TempDir())
    tests := []struct {
        id      string
        wantErr bool
    }{
        {"valid-id-123", false},
        {"cubesandbox-python-slim-3.11-nydus_0", false},  // single dots ok
        {"", true},                                        // empty
        {"foo/bar", true},                                 // forward slash
        {"foo\\bar", true},                                // backslash
        {"foo..bar", true},                                // double dot
        {"../etc/passwd", true},                           // traversal prefix
    }
    for _, tt := range tests {
        _, err := store.path(tt.id)
        if (err != nil) != tt.wantErr {
            t.Errorf("path(%q) err=%v, wantErr=%v", tt.id, err, tt.wantErr)
        }
    }
}

This mirrors the test coverage that already exists for pathutil.ValidateSafeID() in Cubelet/pkg/pathutil/validate_test.go.

Fix this →

@rogeroger-yu Hi, could you please add some unit tests to stabilize this behavior?

@fslongjin okey.

After reviewing this, I don't think this is the right way to fix it. The sandbox ID should be designed as a UUID. The root cause here is that when CubeMaster generates the template, it sends the template OCI image name​ to Cubelet as the sandbox ID.
A possible solution would be to modify CubeMaster to either generate a UUID or pass down the template ID​ to the Cubelet layer.
Note:​ For normal sandbox creation requests, the sandbox ID is generated by Cubelet. However, the current step is creating a sandbox template​ (for app snapshots), which is why CubeMaster specifies the sandbox ID.

@fslongjin I agree with your judgment and resubmit a version. I will review it after my hard work.

cc @fslongjin